[Feat] BannerCard 컴포넌트 제작#25
Hidden character warning
Conversation
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
KyeongJooni
left a comment
There was a problem hiding this comment.
수고하셨습니다! 👍 코멘트 확인해주시면 좋을 것 같아요!
There was a problem hiding this comment.
이미지를 자세히보니 워터마크가 보이는 것 같아요
다른 이미지를 사용해야 할 것 같습니다 피그마에 요청하였으니 추후에 수정 부탁드립니다!
There was a problem hiding this comment.
저도 보면서 뭐지 했는데 감사합니다.! ㅎ
| <p className={title()}> | ||
| 중고 전자기기 | ||
| <br /> | ||
| 구매하기 | ||
| </p> |
There was a problem hiding this comment.
중고 전자기기 구매뿐 아니라 판매 등 역시 같은 컴포넌트를 재사용해야 하기 때문에 하드코딩으로 하는 것보다 props를 받아서 사용하는 건 어떨까요?
type BannerCardProps = {
title: string;
description: string;
buttonText?: string;
onClick?: () => void;
}이런식으로 타입 정의를 하여 props로 받아서 사용하면 더 좋을 것 같습니다!
There was a problem hiding this comment.
옙 props로 받아서 사용하는 게 훨씬 나은 것 같아 수정 완료했습니다.!
| <div | ||
| data-testid="banner-card" | ||
| className={base({ className })} | ||
| role="button" | ||
| tabIndex={0} | ||
| onClick={onClick} | ||
| onKeyDown={(e) => { | ||
| if (e.key === 'Enter' || e.key === ' ') { | ||
| e.preventDefault(); | ||
| onClick?.(); | ||
| } | ||
| }} | ||
| {...props} | ||
| > |
There was a problem hiding this comment.
버튼 클릭 시 stopPropagation()으로 부모 이벤트 멈추고, 다시 onClick() 메소드를 호출하고 있는 상황 같은데 바로가기를 눌렀을 때 이동할 것인지 카드 전체를 눌렀을 때 이동할 것인지 고려하고 한쪽은 onClick() 지워주시면 좋을 것 같아요!
There was a problem hiding this comment.
뭔가 카드 클릭, 버튼 클릭하여 이동하는 것을 모두 허용하게 하려 해서 약간 꼬였던 것 같습니다! 감사합니다!
| <Button | ||
| size="auto" | ||
| className="w-26" | ||
| onClick={(e) => { | ||
| e.stopPropagation(); | ||
| onClick?.(); | ||
| }} | ||
| > |
There was a problem hiding this comment.
이 부분에서 stopPropagation을 하여 막고 있는 것 같습니다!
|
|
||
| <Button | ||
| size="auto" | ||
| className="w-26" |
There was a problem hiding this comment.
The class w-[104px] can be written as w-26 이렇게 떠서 바꿔봤는데 변경했습니다!
There was a problem hiding this comment.
정의된 디자인 토큰에 있는 값들은 디자인 토큰 값을 사용하면 좋을 것 같아요!
그리고 현재 Card와 BannerCard 두 컴포넌트에서 tv() + slots만 사용 중인데, variants 없이 tv() 쓰는 건 오버엔지니어링이라고 개인적으로 생각하고 있어요 개인적인 생각이기 때문에 참고만 해주세요! 😄
| 'flex', | ||
| 'flex-col', | ||
| 'items-start', | ||
| 'gap-[12px]', |
| 'transition-shadow', | ||
| 'hover:shadow-md', | ||
| 'focus-visible:ring-2', | ||
| 'focus-visible:ring-[var(--color-green-700)]', |
There was a problem hiding this comment.
피그마 디자인에 명세가 되어있는 속성들일까요? 확인부탁드립니다!
There was a problem hiding this comment.
어디서 나온 친구인지 몰라 삭제 했습니다!
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
| 'group-hover:w-[307px]', | ||
| 'group-hover:h-[307px]', | ||
| 'group-hover:w-[307px]', | ||
| 'group-hover:h-[307px]', |
There was a problem hiding this comment.
이 부분 중복 선언하신 특별한 이유가 있으신가요?? 없으시다면 정리하는 게 좋을 것 같습니다!
There was a problem hiding this comment.
수정하다가 중복으로 들어간 것 같습니다! 당장 수정하겠습니다! 감사합니다!
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
PR 검증 결과✅ TypeScript: 통과 |
✨ 주요 변경사항
📝 작업 상세 내용
✅ 체크리스트
Close #번호추가📸 스크린샷 (선택)
🔍 기타 참고사항
버튼 클릭 시 이벤트 중복 호출 문제가 있었는데 수정했습니다.
🔗 관련 이슈